Copy executables to Python virtual environment#500
Open
Game4Move78 wants to merge 2 commits intoActivityWatch:masterfrom
Open
Copy executables to Python virtual environment#500Game4Move78 wants to merge 2 commits intoActivityWatch:masterfrom
Game4Move78 wants to merge 2 commits intoActivityWatch:masterfrom
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d885b30 in 1 minute and 57 seconds
More details
- Looked at
87lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. Makefile:5
- Draft comment:
Consider using 'python3' instead of 'python' to ensure consistency on systems where Python 3 is required. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While using python3 explicitly can be good practice, this seems like an implementation detail that would be caught immediately if it was an issue. If 'python' doesn't point to Python 3 on the system, the build would fail immediately and obviously. Most modern systems alias python to python3 by default. The comment doesn't indicate a clear problem that needs fixing.
The comment could be valid if the project has specific Python 3 requirements or if there are systems where this could cause compatibility issues.
If Python 3 was required, the build would fail immediately and obviously. This is more of a system configuration issue than a code issue.
Delete the comment as it's not addressing a clear problem that needs fixing. The build system would catch any Python version incompatibility immediately.
2. Makefile:111
- Draft comment:
Consider using 'python3' to call the cleanup script for consistency with the build target. - Reason this comment was not posted:
Marked as duplicate.
3. scripts/copy_rust_binaries.py:11
- Draft comment:
In is_executable() consider checking the OS first to simplify the logic for Windows-specific extensions. - Reason this comment was not posted:
Confidence changes required:50%
None
4. Makefile:5
- Draft comment:
Use a version-specific interpreter (e.g. 'python3' or a $(PYTHON) variable) instead of 'python' to avoid ambiguity on systems where 'python' might point to Python 2. - Reason this comment was not posted:
Marked as duplicate.
5. Makefile:112
- Draft comment:
Use 'python3' (or a defined $(PYTHON) variable) instead of 'python' here as well to ensure the correct interpreter is used. - Reason this comment was not posted:
Marked as duplicate.
6. scripts/copy_rust_binaries.py:57
- Draft comment:
Specify nargs='?' for the positional 'target_dir' argument so that the default value is used when omitted. - Reason this comment was not posted:
Marked as duplicate.
7. scripts/copy_rust_binaries.py:38
- Draft comment:
On Windows, the is_executable() fallback does not check for file existence. Verify the file exists before calling os.remove() to avoid errors. - Reason this comment was not posted:
Comment did not seem useful.
8. scripts/copy_rust_binaries.py:20
- Draft comment:
Consider printing the error message to sys.stderr instead of stdout for better error reporting. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_JcNlOaEWeOdtGKmm
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
scripts/copy_rust_binaries.py
Outdated
| parser = argparse.ArgumentParser() | ||
|
|
||
| parser.add_argument("--clean", action='store_true', default=False) | ||
| parser.add_argument("target_dir", type=str, default=os.path.join("target", "release")) |
There was a problem hiding this comment.
Positional argument 'target_dir' provides a default value but lacks nargs='?'; this may prevent the default from being used if omitted.
scripts/copy_rust_binaries.py
Outdated
| def build(target_dir, python_bin_dir): | ||
|
|
||
| if not os.path.exists(target_dir): | ||
| print(f"Error: {target_dir} does not exist. Did you run `cargo build --release`?") |
There was a problem hiding this comment.
Consider writing the error message to stderr instead of stdout when the target directory doesn't exist.
Game4Move78
force-pushed
the
copy-target-venv
branch
from
d885b30 to
9f526fa
Compare
Author
|
The alternative would be to update the docs with this: Build and install everything into the virtualenv:
.. code-block:: sh
make build
cp aw-server-rust/target/release/aw-server venv/bin/aw-server-rust # optional
.. note::
If you want to use `aw-server-rust` with `aw-qt`, you'll want to copy its binary into your `venv` |
Member
|
This is essentially what |
Author
|
Rationale:
If you don't like this way of moving the binaries, would you prefer using Poetry to install them? |
Author
|
Or use Poetry to install a small Python script that invokes the rust executable? |
Game4Move78
force-pushed
the
copy-target-venv
branch
4 times, most recently
from
8cb42d8 to
4a3b0b6
Compare
Game4Move78
force-pushed
the
copy-target-venv
branch
from
4a3b0b6 to
691a4ab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This sets the Makefile to copy
aw-server-rust(and other executables appending -rust suffix to basename) to the virtualenv (or Python scripts folder) to make the build process more intuitive. Allowsaw-server-rustto be run from terminal when the Python environment (i.e. virtualenv) used to install ActivityWatch is active, and simplifies usingaw-qtwithaw-server-rustwithout added documentation and extra steps.As it is the user has to go into
aw-server-rust, docp target/release/aw-server ../venv/bin/aw-server-rustmanually per the README in order to getaw-server-rustavailable underaw-qt. Or the rust server is executed in a different path from other modules.Related to ActivityWatch/aw-qt#107 as aw-server-rust is now the preferred server, which is not documented yet.
Important
Automates copying of Rust executables to Python virtual environment in the build process, simplifying integration with
aw-qt.Makefileupdated to runscripts/copy_rust_binaries.pyduringbuildandcleantargets.-rustto the basename.aw-server-rustis available in the Python environment when active.scripts/copy_rust_binaries.pyadded.build()function copies executables fromtarget_dirto Python scripts directory.clean()function removes copied executables from Python scripts directory.sysconfigto determine Python scripts directory.aw-server-rustthe preferred server inaw-qt.This description was created by
for d885b30. It will automatically update as commits are pushed.